Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple configuration store from scraping service #445

Merged
merged 2 commits into from
Mar 3, 2021

Conversation

rfratto
Copy link
Member

@rfratto rfratto commented Mar 2, 2021

PR Description

This PR starts the work of decoupling the configuration store from the scraping service. This will eventually be needed to make the KV-based configuration store optional, but is more immediately useful to /-/reload (#147) where we may want to dynamically update the configuration for a store.

Which issue(s) this PR fixes

Related to #147.

Notes to the Reviewer

Not hooked in yet. I didn't want to delete too much code from pkg/prom/ha so this didn't become too hard to review.

The HTTP API for the config store would move to this new package too.

Tagging @mattdurham and @56quarters to get some early thoughts. This will be a pretty drastic change from how the scraping service was handled before.

@rfratto rfratto force-pushed the configstore branch 2 times, most recently from edb116f to a01ab53 Compare March 2, 2021 22:18
@rfratto rfratto marked this pull request as ready for review March 3, 2021 14:32
@rfratto
Copy link
Member Author

rfratto commented Mar 3, 2021

Taking this out of draft. I think we can merge it in this state and follow up with a few more PRs to hook it in.

pkg/prom/instance/configstore/remote.go Show resolved Hide resolved
if otherConfig.Name == cfg.Name {
continue
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in love with this, but I cant think of a better way to do it with the default set of golang collections.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the uniqueness checking at all honestly. We have #398 in the backlog to try to avoid this altogether

@rfratto rfratto merged commit d9c8680 into grafana:main Mar 3, 2021
@rfratto rfratto deleted the configstore branch March 3, 2021 15:31
@rfratto rfratto mentioned this pull request Mar 3, 2021
3 tasks
@mattdurham mattdurham mentioned this pull request Sep 7, 2021
3 tasks
mattdurham pushed a commit that referenced this pull request Nov 11, 2021
* initial commit of a configstore

* review feedback
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Apr 17, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants